Skip to content

New Resource: azurerm_data_protection_backup_policy_data_lake_storage#31179

Merged
sreallymatt merged 37 commits intohashicorp:mainfrom
neil-yechenwei:dataprotectionbackuppolicyadlsstorage
Mar 30, 2026
Merged

New Resource: azurerm_data_protection_backup_policy_data_lake_storage#31179
sreallymatt merged 37 commits intohashicorp:mainfrom
neil-yechenwei:dataprotectionbackuppolicyadlsstorage

Conversation

@neil-yechenwei
Copy link
Copy Markdown
Contributor

@neil-yechenwei neil-yechenwei commented Nov 24, 2025

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR is to support Azure Data Lake Storage (ADLS) Policy under Backup Vault mentioned in #31156

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevant documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
--- PASS: TestAccDataProtectionBackupPolicyDataLakeStorage_basic (208.73s)
--- PASS: TestAccDataProtectionBackupPolicyDataLakeStorage_requiresImport (195.15s)
--- PASS: TestAccDataProtectionBackupPolicyDataLakeStorage_complete (223.06s)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • New Resource: azurerm_data_protection_backup_policy_data_lake_storage

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes # 0000

AI Assistance Disclosure

  • AI Assisted - This contribution was made by, or with the assistance of, AI/LLMs

Rollback Plan

If a change needs to be reverted, we will publish an updated version of the provider.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

ForceNew: true,
ValidateFunc: validation.StringMatch(
regexp.MustCompile("^[-a-zA-Z0-9]{3,150}$"),
"DataProtection BackupPolicy name must be 3 - 150 characters long, contain only letters, numbers and hyphens.",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"DataProtection BackupPolicy name must be 3 - 150 characters long, contain only letters, numbers and hyphens.",
"`name` must be 3 - 150 characters long, contain only letters, numbers and hyphens(-).",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"life_cycle": {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since life_cycle is the only nested-property of default_retention_rule, can it be flattened?

Copy link
Copy Markdown
Contributor Author

@neil-yechenwei neil-yechenwei Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All existing similar resources under https://github.com/hashicorp/terraform-provider-azurerm/tree/main/internal/services/dataprotection already use this structure for that property, so keeping it consistent with the style of other resources would be better, making it easier for users to use and understand.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with HC: please flatten this nested prop because it only contains 1 inner prop. See guide: Flattening nested properties in schema-design-considerations.md. It's ok to deviate from the other backup_policy resources.

Please also drop the lifecycle term from the name since it does not appear anywhere in portal.

This block should look like:

default_retention_rule {
  duration = "P7D"
}

Note that we cannot flatten "duration" because we want to reserve possibility to add "data_store_type" in the future.

Comment on lines +262 to +268
var model BackupPolicyDataLakeStorageModel
if err := metadata.Decode(&model); err != nil {
return fmt.Errorf("decoding: %+v", err)
}

client := metadata.Client.DataProtection.BackupPolicyClient
subscriptionId := metadata.Client.Account.SubscriptionId
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var model BackupPolicyDataLakeStorageModel
if err := metadata.Decode(&model); err != nil {
return fmt.Errorf("decoding: %+v", err)
}
client := metadata.Client.DataProtection.BackupPolicyClient
subscriptionId := metadata.Client.Account.SubscriptionId
client := metadata.Client.DataProtection.BackupPolicyClient
subscriptionId := metadata.Client.Account.SubscriptionId
var model BackupPolicyDataLakeStorageModel
if err := metadata.Decode(&model); err != nil {
return fmt.Errorf("decoding: %+v", err)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


* `vault_id` - (Required) The ID of the Backup Vault where the Azure Backup Policy Data Lake Storage should exist. Changing this forces a new resource to be created.

* `backup_repeating_time_intervals` - (Required) Specifies a list of repeating time interval. It supports weekly back. It should follow `ISO 8601` repeating time interval format. Changing this forces a new resource to be created.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `backup_repeating_time_intervals` - (Required) Specifies a list of repeating time interval. It supports weekly back. It should follow `ISO 8601` repeating time interval format. Changing this forces a new resource to be created.
* `backup_repeating_time_intervals` - (Required) Specifies a list of repeating time interval. It supports weekly backup. It should follow `ISO 8601` repeating time interval format. Changing this forces a new resource to be created.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


## Import

Azure Backup Policy Data Lake Storage's can be imported using the `resource id`, e.g.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Azure Backup Policy Data Lake Storage's can be imported using the `resource id`, e.g.
Azure Backup Policy Data Lake Storages can be imported using the `resource id`, e.g.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}
resp, err := client.DataProtection.BackupPolicyClient.Get(ctx, *id)
if err != nil {
return nil, fmt.Errorf("reading %s: %+v", *id, err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("reading %s: %+v", *id, err)
return nil, fmt.Errorf("retrieving %s: %+v", *id, err)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

if err != nil {
return nil, fmt.Errorf("reading %s: %+v", *id, err)
}
return utils.Bool(resp.Model != nil), nil
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return utils.Bool(resp.Model != nil), nil
return pointer.To(resp.Model != nil), nil

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines +80 to +82
provider "azurerm" {
features {}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move the provider definition into each testcase?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


// API has bug, which appears API returns before the resource is fully deleted. Tracked by this issue: https://github.com/Azure/azure-rest-api-specs/issues/38944
log.Printf("[DEBUG] Waiting for %s to be fully deleted..", *id)
stateConf := &pluginsdk.StateChangeConf{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pluginsdk.StateChangeConf is deprecated. Could you use poller instead? (ref)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@neil-yechenwei
Copy link
Copy Markdown
Contributor Author

@ms-zhenhua , thanks for the comments. I updated PR. Please take another look. Below is the latest test result.

image

Copy link
Copy Markdown
Collaborator

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM~

@neil-yechenwei neil-yechenwei marked this pull request as ready for review December 8, 2025 02:01
Copy link
Copy Markdown
Collaborator

@liuwuliuyun liuwuliuyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments for small changes. Thanks

}

func (r DataProtectionBackupPolicyDataLakeStorageResource) Arguments() map[string]*pluginsdk.Schema {
arguments := map[string]*pluginsdk.Schema{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider reordering the fields in the Arguments() function to follow the standard pattern: Required fields first (alphabetically but name first), then Optional fields (alphabetically).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@neil-yechenwei
Copy link
Copy Markdown
Contributor Author

@liuwuliuyun , thanks for the comments. I updated PR. Please take another look.

@ziyeqf
Copy link
Copy Markdown
Collaborator

ziyeqf commented Mar 17, 2026

image


* `weeks_of_month` - (Optional) Possible values are `First`, `Second`, `Third`, `Fourth` and `Last`. Changing this forces a new resource to be created.

-> **Note:** When not using `absolute_criteria`, you must use exactly one of `days_of_month` or `days_of_week`. Regarding the remaining two properties, `weeks_of_month` and `months_of_year`, you may use either, both, or neither. If you would like to set multiple intervals, you may do so by using multiple `retention_rule` blocks.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

days_of_month argument does not exist


* `retention_rule` - (Optional) One or more `retention_rule` blocks as defined below. The priority of each rule is determined by its order in the list, where the first rule has the highest priority. Changing this forces a new resource to be created.

* `time_zone` - (Optional) Specifies the Time Zone which should be used by the backup schedule. Changing this forces a new resource to be created.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we provide a link to list of valid values? A link to the internal/services/dataprotection/validate/backup_policy_data_lake_storage_time_zone.go file it will be very helpful to our users.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find a proper link to add here, and not thinking add a link to go file is good choice.. So added the long list there as possible values.

Status: pollers.PollingStatusSucceeded,
}
pollingInProgress = pollers.PollResult{
HttpResponse: nil,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AZBP006: redundant nil assignment to pointer field "HttpResponse" - omit the field

Got this error from azurerm-linter. There are 6 other violations. Please run the linter on your local and fix / add explanation if you disagree with the linters.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❯ azurerm-linter --base main --remote hashi
2026/03/23 03:59:27 Using local git diff mode
2026/03/23 03:59:27 Current branch: dataprotectionbackuppolicyadlsstorage
2026/03/23 03:59:27 Merge-base with hashi/main: 9a03001
2026/03/23 03:59:27 ✓ Found 7 changed files with 968 changed lines
2026/03/23 03:59:27 Changed lines filter: tracking 7 files with 968 changed lines
2026/03/23 03:59:27 Auto-detected 1 changed packages:
2026/03/23 03:59:27 ./internal/services/dataprotection/...
2026/03/23 03:59:27 Loading packages...
2026/03/23 03:59:37 Running analysis...
2026/03/23 03:59:39 ✓ Analysis completed successfully with no issues found

Copy link
Copy Markdown
Collaborator

@gerrytan gerrytan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ziyeqf there are some more concerns I missed from the previous review. This PR looks pretty good overall though.

},
},
},
},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weeks_of_month, scheduled_backup_times, months_of_year, days_of_week and absolute_criteria are all optional. Is it valid to create a retention_rule with just name and duration?

Maybe this needs an AtLeastOneOf?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AtLeastOneOf does not support well for elements in a list.. added a validation logic in the create() function

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, this always trips me. I remember you've tested the API error is descriptive enough. I think this is fine. Thanks for adding the validation.


* `weeks_of_month` - (Optional) Possible values are `First`, `Second`, `Third`, `Fourth` and `Last`. Changing this forces a new resource to be created.

-> **Note:** When not using `absolute_criteria`, you must use exactly one of `weeks_of_month` or `days_of_week`. Regarding the remaining two properties, `weeks_of_month` and `months_of_year`, you may use either, both, or neither. If you would like to set multiple intervals, you may do so by using multiple `retention_rule` blocks.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer passive grammar, avoid using 'you'

Suggested change
-> **Note:** When not using `absolute_criteria`, you must use exactly one of `weeks_of_month` or `days_of_week`. Regarding the remaining two properties, `weeks_of_month` and `months_of_year`, you may use either, both, or neither. If you would like to set multiple intervals, you may do so by using multiple `retention_rule` blocks.
-> **Note:** When not using `absolute_criteria`, exactly one of `weeks_of_month` or `days_of_week` must be used. `weeks_of_month` and `months_of_year` are optional, both can be supplied together. Multiple intervals may be set using multiple `retention_rule` blocks.

ValidateFunc: azValidate.ISO8601Duration,
},

"data_protection_backup_vault_id": commonschema.ResourceIDReferenceRequiredForceNew(pointer.To(basebackuppolicyresources.BackupVaultId{})),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a parent id, it has to be ordered after name. Please also reorder the markdown doc.

Refer to guide-new-resource.md:169

Comment on lines +120 to +121
backup_schedule = ["R/2021-05-23T02:30:00+00:00/P1W"]
default_retention_duration = "P4M"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep these tied to the original resource

Suggested change
backup_schedule = ["R/2021-05-23T02:30:00+00:00/P1W"]
default_retention_duration = "P4M"
backup_schedule = azurerm_data_protection_backup_policy_data_lake_storage.test.backup_schedule
default_retention_duration = azurerm_data_protection_backup_policy_data_lake_storage.test.default_retention_duration

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unresolved

Copy link
Copy Markdown
Collaborator

@gerrytan gerrytan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing all the feedbacks @ziyeqf , I have triggered another acctest run: https://hashicorp.teamcity.com/viewQueued.html?itemId=636263

Assuming the result is good, this PR is good, nice work! 👍

@ziyeqf
Copy link
Copy Markdown
Collaborator

ziyeqf commented Mar 26, 2026

That test failed with network issue, here is the re-run result:
image

Comment on lines +120 to +121
backup_schedule = ["R/2021-05-23T02:30:00+00:00/P1W"]
default_retention_duration = "P4M"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unresolved

Comment on lines +88 to +94
* `absolute_criteria` - (Optional) Possible values are `AllBackup`, `FirstOfDay`, `FirstOfWeek`, `FirstOfMonth` and `FirstOfYear`. These values mean the first successful backup of the day/week/month/year. Changing this forces a new resource to be created.

* `days_of_week` - (Optional) Possible values are `Monday`, `Tuesday`, `Wednesday`, `Thursday`, `Friday`, `Saturday` and `Sunday`. Changing this forces a new resource to be created.

* `weeks_of_month` - (Optional) Possible values are `First`, `Second`, `Third`, `Fourth` and `Last`. Changing this forces a new resource to be created.

* `months_of_year` - (Optional) Possible values are `January`, `February`, `March`, `April`, `May`, `June`, `July`, `August`, `September`, `October`, `November` and `December`. Changing this forces a new resource to be created.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing some form of a description for these properties. E.g. The days of the week on which to retain a backup (I think that's what this does?)

Copy link
Copy Markdown
Collaborator

@sreallymatt sreallymatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those final changes @ziyeqf - LGTM ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants